Skip to content

[HybridWebView] Fix some issues with the interception, typescript and other features#29829

Merged
rmarinho merged 4 commits intonet10.0from
dev/hybridwebview-header-case
Jun 5, 2025
Merged

[HybridWebView] Fix some issues with the interception, typescript and other features#29829
rmarinho merged 4 commits intonet10.0from
dev/hybridwebview-header-case

Conversation

@mattleibow
Copy link
Member

@mattleibow mattleibow commented Jun 4, 2025

This pull request introduces several improvements and fixes to the HybridWebView component, focusing on case-insensitive header handling, improving compatibility across platforms, refactoring JavaScript code, and enhancing testing. The most significant changes include updates to header dictionaries to support case-insensitivity, refactoring the JavaScript code for better readability and maintainability, and adding platform-specific compatibility checks in tests.

Case-insensitive header handling:

JavaScript code refactoring:

  • src/Core/src/Handlers/HybridWebView/HybridWebView.js: Refactored JavaScript functions such as SendRawMessage, InvokeDotNet, and __InvokeJavaScript into standalone functions, and restructured the HybridWebView object for better readability and maintainability. Removed optional chaining for compatibility. [1] [2] [3] [4]

Platform-specific compatibility in tests:

TypeScript updates:

These changes collectively enhance the HybridWebView component's functionality, improve cross-platform compatibility, and ensure robust testing practices.

Android: Observed lower case on some OS versions (ie 28)
Windows: Header lookup is case insensitive
macOS/iOS: CI will inform
Copilot AI review requested due to automatic review settings June 4, 2025 21:26
@mattleibow mattleibow requested a review from a team as a code owner June 4, 2025 21:26
@mattleibow mattleibow requested review from PureWeen and rmarinho June 4, 2025 21:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new test case to verify that the HybridWebView can intercept requests and read headers in a case-insensitive manner, addressing issues observed on various OS versions.

  • Added a new test method RequestsCanBeInterceptedAndCaseInsensitiveHeadersRead.
  • Incorporated conditional test data for different platforms based on header handling capabilities.
Comments suppressed due to low confidence (1)

src/Controls/tests/DeviceTests/Elements/HybridWebView/HybridWebViewTests.cs:771

  • [nitpick] Consider refactoring the repetitive try-catch blocks used for header value retrieval into a helper function or loop to reduce duplication and improve maintainability.
try { headerValues["X-Echo-Name"] = e.Headers["X-Echo-Name"]; }

@mattleibow
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow mattleibow changed the title [HybridWebView] Sometimes the OS changes the header casing [HybridWebView] Fix some issues with the interception, typescript and other features Jun 5, 2025
Comment on lines -56 to -58
internal IReadOnlyDictionary<string, string>? GetRequestHeaders() => new WrappedHeadersDictionary(Request.Headers);

class WrappedHeadersDictionary : IReadOnlyDictionary<string, string>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous way to use wrappers was me being "so clever" and "avoiding allocations" and "reducing copies"... But yeah, that was not sensible since the amount of things that would happen was so little. In order to hit this path, the user has to actually request the headers.

For all other cases, this doesn't do anything.

Comment on lines +185 to +188
internal IReadOnlyDictionary<string, string> GetRequestHeaders() =>
_headers ??= Request.RequestHeaders is { } rh
? new Dictionary<string, string>(rh, StringComparer.OrdinalIgnoreCase)
: new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, depending on the server and how the headers are populated, Android sometimes converts the headers to lower case. So we always will ned a case-insensitive.

Windows was already case insensitive, so this just makes it all consistent.

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests now pass!

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked too soon , seems still have some failure on API24, but it doesn t crash anymore

Screenshot 2025-06-05 at 11 04 04

}
// Determine the mechanism to receive messages from the host application.
if (window.chrome?.webview?.addEventListener) {
if (window.chrome && window.chrome.webview && window.chrome.webview.addEventListener) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Android Chromeis weird. It suports async functions ahead of its time, but then also does not support ?.. I am at a loss.

@rmarinho
Copy link
Member

rmarinho commented Jun 5, 2025

/backport to release/10.0.1xx-preview5

@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2025

Started backporting to release/10.0.1xx-preview5: https://github.com/dotnet/maui/actions/runs/15473805094

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok seems we are green now! Thanks @mattleibow

@rmarinho
Copy link
Member

rmarinho commented Jun 5, 2025

I backported but I also have this branch to just bring latest changes to the release branch .. go wild or go home

@rmarinho rmarinho merged commit e79ada6 into net10.0 Jun 5, 2025
125 of 130 checks passed
@rmarinho rmarinho deleted the dev/hybridwebview-header-case branch June 5, 2025 18:47
@mattleibow
Copy link
Member Author

/backport to main

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2025

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2025

@mattleibow backporting to "main" failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Sometimes the OS changes the header casing
Using index info to reconstruct a base tree...
M	src/Controls/tests/DeviceTests/Elements/HybridWebView/HybridWebViewTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Controls/tests/DeviceTests/Elements/HybridWebView/HybridWebViewTests.cs
CONFLICT (content): Merge conflict in src/Controls/tests/DeviceTests/Elements/HybridWebView/HybridWebViewTests.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Sometimes the OS changes the header casing
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants